Skip to content

Conversation

@hnrklssn
Copy link
Member

This creates safe overloads for any methods in the protocol annotated with bounds information.
To support code sharing for both clang::FunctionDecl and clang::ObjCMethodDecl, swiftifyImpl is templated.
Updates _SwiftifyImportProtocol to make the added overloads in the protocol public.

rdar://144335990

...with bounds attributes

This creates safe overloads for any methods in the protocol annotated
with bounds information. Updates _SwiftifyImportProtocol to make the
added overloads in the protocol public.

rdar://144335990
@hnrklssn
Copy link
Member Author

@swift-ci please smoke test

@hnrklssn
Copy link
Member Author

@swift-ci please smoke test

This updates SwiftifyImportProtocolPrinter such that it no longer emits
anything for methods without bounds or lifetime info. Previously we
would not attach the macro if no methods in the protocol contained
bounds or lifetime info. However if one of the methods did, we would
still emit `.method(signature: "func foo()", paramInfo: [])` for the
methods without bounds of lifetime info. This would result in overloads
being generated, like so:
```
@_alwaysEmitIntoClient @_disfavoredOverload public
func foo() {
  return unsafe foo()
}
```

As part of this change, SwiftifyImportPrinter is now an abstract parent
type for SwiftifyImportProtocolPrinter, and the new
SwiftifyImportFunctionPrinter. Instead of SwiftifyImportProtocolPrinter
inheriting the function printing, `printMethod` instead creates a new
SwiftifyImportFunctionPrinter each time, with a new output string. If it
output anything interesting the result is forwarded, otherwise it is
discarded.
@hnrklssn
Copy link
Member Author

@swift-ci please smoke test

1 similar comment
@hnrklssn
Copy link
Member Author

@swift-ci please smoke test

@hnrklssn
Copy link
Member Author

@swift-ci please smoke test windows platform

1 similar comment
@hnrklssn
Copy link
Member Author

@swift-ci please smoke test windows platform

@hnrklssn
Copy link
Member Author

@swift-ci please smoke test

@hnrklssn
Copy link
Member Author

hnrklssn commented Nov 5, 2025

For a second I was questioning whether [Swiftify] make protocol extension methods public was the right way to do things.
After looking into it a bit, my understanding is that if you can name an objc method, you can call it, so their Swift equivalent should always be public IIUC. Not sure why they aren't explicitly public in the imported protocol, but Swift still seems to be able to call those methods, so maybe there's some compiler magic? @egorzhdan @DougGregor

@hnrklssn
Copy link
Member Author

hnrklssn commented Nov 7, 2025

ping @egorzhdan @j-hui @DougGregor

Copy link
Contributor

@j-hui j-hui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, aside from a couple of nits. But I'm not very familiar with how ObjC protocols are imported so would prefer if this were reviewed by someone else too.

Comment on lines +1811 to +1815
let hasVisibilityModifier = method.modifiers.contains { modifier in
let modName = modifier.name.trimmed.text
return modName == "public" || modName == "internal" || modName == "open"
|| modName == "private" || modName == "filePrivate"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the scenario is reachable, but you're missing package, and filePrivate is spelled all lowercase. I think you can simplify this to:

Suggested change
let hasVisibilityModifier = method.modifiers.contains { modifier in
let modName = modifier.name.trimmed.text
return modName == "public" || modName == "internal" || modName == "open"
|| modName == "private" || modName == "filePrivate"
}
let hasVisibilityModifier = method.modifiers.contains {
return switch $0.name.trimmed.text {
case "open", "public", "package", "internal", "fileprivate", "private": true
default: false
}
}

Comment on lines +1377 to +1378
else // decls imported from clang do not have a SourceFile
ASSERT(isa<FileUnit>(dc) && !isa<SourceFile>(dc));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: put the comment in the assertion

Suggested change
else // decls imported from clang do not have a SourceFile
ASSERT(isa<FileUnit>(dc) && !isa<SourceFile>(dc));
else
ASSERT(isa<FileUnit>(dc) && !isa<SourceFile>(dc) && "decls imported from Clang should not have a SourceFile");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants